Skip to content

feat(skills): add local skill CLI support#1162

Open
anandh8x wants to merge 21 commits into
Gitlawb:mainfrom
anandh8x:skill-hub-local-skills-cli
Open

feat(skills): add local skill CLI support#1162
anandh8x wants to merge 21 commits into
Gitlawb:mainfrom
anandh8x:skill-hub-local-skills-cli

Conversation

@anandh8x
Copy link
Copy Markdown
Collaborator

@anandh8x anandh8x commented May 14, 2026

Summary

  • add local skills CLI commands for listing, showing, validating, and removing skills
  • support project/user/bundled skill discovery with project precedence
  • keep skills CLI output free of the startup banner and format list output as a readable table
  • add JSON output for skills list metadata

Scope

This covers the OpenClaude CLI read/manage portion of the Skill Hub. Registry install, the separate skills registry repo, and website catalog work are intentionally left for follow-up PRs.

Tests

  • bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts
  • bun run build

@anandh8x
Copy link
Copy Markdown
Collaborator Author

Correction to the CI follow-up comment above: the shell expanded Markdown backticks while posting it, so here is the clean version.

CI follow-up update:

  • Fixed the PR-added skills tests so they pass under the full Bun CI suite:
    • switched the new skills formatter test to bun:test
    • isolated the user-vs-project skill precedence assertion from shared process config state
  • The skills-related tests are now passing in the GitHub Actions log.
  • Also stabilized src/utils/conversationArc.perf.test.ts, which was failing CI because it used persisted shared knowledge-graph state plus strict wall-clock benchmark thresholds on a shared runner. The benchmark now uses an isolated temp config directory and coarse regression limits.

Local verification before pushing the latest fix:

bun test src/utils/conversationArc.perf.test.ts src/skills/loadSkillsDir.test.ts src/cli/handlers/skills.test.ts src/commands.test.ts
bun run smoke

web is passing. smoke-and-tests is currently rerunning after the latest push.

Copy link
Copy Markdown
Collaborator

@Vasanthdev2004 Vasanthdev2004 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing this forward. I reviewed the current head as a targeted Skill Hub CLI review, focusing on local/registry install behavior, skill discovery precedence, validation, banner suppression, and the new list/show/remove flow.

Verdict: Needs changes

Blocking issue:

  1. The install flow needs to validate/sanitize the skill name before using it in filesystem paths.

In src/cli/handlers/skillsInstall.ts, prepareSkillFromMarkdown() can take registryEntry.name or SKILL.md frontmatter name, then immediately builds tempDir = join(tempRoot, skillName). Later cleanup removes dirname(candidate.tempDir). If a registry entry or raw SKILL.md uses a path-like name such as .. or ../x, the temp path and cleanup target can escape the intended temp skill folder before validateSkillPath() reports the invalid name.

Please fix this before merge by making the install path construction safe before validation runs. A good shape would be:

  • validate/normalize the candidate skill name before any join(tempRoot, skillName) or install target path is created
  • keep the original tempRoot as a separate field and remove that exact directory in finally, instead of deriving cleanup from dirname(candidate.tempDir)
  • verify the final target directory resolves inside the intended skills root before rm/cp, especially for --force
  • add a regression test for an invalid/path-traversal skill name from a raw SKILL.md or registry entry

What I checked:

  • skills list/show/validate/install/remove command shape
  • .openclaude/skills vs legacy .claude/skills precedence
  • banner suppression for skills management commands
  • local directory and registry install tests
  • CI status

Local verification I ran:

  • bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts
  • bun run build
  • git diff --check

Those checks passed, and the overall direction is good. I just do not want this install path to merge until the pre-validation filesystem boundary is hardened.

Non-blocking note: registry installs currently only enforce sha256 when metadata provides it. For the Skill Hub trust model, I’d prefer requiring hashes for the default registry path, or at least making no-hash registry installs an explicit maintainer decision.

@anandh8x
Copy link
Copy Markdown
Collaborator Author

Addressed the requested install path hardening in commit 361a4ac.

Changes made:

  • validate the install skill name before any temp or target path is constructed
  • keep the temp root as an explicit cleanup target instead of deriving it from the candidate path
  • resolve final install targets under the selected skills root before copy/force overwrite
  • added regression coverage for path-like names from both raw SKILL.md and registry metadata

Verification:

  • bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts
  • bun run build
  • bun run smoke
  • git diff --check
  • GitHub Actions: smoke-and-tests and web are passing on the latest run.

Vasanthdev2004
Vasanthdev2004 previously approved these changes May 14, 2026
Copy link
Copy Markdown
Collaborator

@Vasanthdev2004 Vasanthdev2004 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Targeted re-review of the latest Skill Hub CLI fixes.

Verdict: Approve-ready

What I rechecked:

  • the previous install-path blocker around unsafe skill names
  • temp directory cleanup behavior
  • target path containment before rm / cp
  • raw SKILL.md and registry path-traversal regression coverage
  • local/project/global skills CLI flow at the code level
  • current CI status

The blocker I raised is fixed on the current head. Skill names are normalized before path construction, cleanup now removes the explicit temp root, install targets are resolved under the intended skills root, and the new regression tests cover the unsafe raw-markdown and registry-name cases.

Local verification I ran:

  • bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts
  • bun run build
  • bun run smoke
  • git diff --check

All passed. I do not see a remaining blocker from my side for this PR scope.

Small non-blocking note: colon namespace skill names may be awkward as literal install folder names on Windows, but that is not the same safety issue and can be handled separately if it becomes a UX problem.

@Vasanthdev2004
Copy link
Copy Markdown
Collaborator

good for me @anandh8x we just need to test all 3 now

@anandh8x anandh8x marked this pull request as ready for review May 14, 2026 11:28
@anandh8x anandh8x marked this pull request as draft May 14, 2026 14:11
@Vasanthdev2004
Copy link
Copy Markdown
Collaborator

Pushed a maintainer follow-up onto this branch to get it current with main and tighten the install-side trust boundary.

What changed:

  • resolved the main merge conflicts; PR is mergeable again
  • registry installs now require a sha256 pin and refuse unpinned registry entries
  • registry installs still verify the downloaded SKILL.md content against the registry hash before writing anything
  • min_openclaude_version is enforced at install time
  • tools_required and min_openclaude_version are preserved into installed skill.json
  • non-official/non-standard trust tiers now print a warning during install

What I intentionally did not add here:

  • session-start revocation checks
  • runtime tool permission prompts
  • trust promotion governance

Those are bigger runtime/governance pieces and should land as focused follow-ups after this local skills CLI foundation is merged.

Verification run locally:

  • bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts
  • bun test src/utils/conversationArc.perf.test.ts src/utils/knowledgeGraph.stress.test.ts src/utils/providerProfile.test.ts
  • bun run build
  • bun run smoke

Note: bun run lint no longer exists on current main. bun run typecheck still fails with broad existing repo type errors unrelated to this PR, so I did not treat that as a blocker for this branch.

@Vasanthdev2004 Vasanthdev2004 marked this pull request as ready for review May 15, 2026 10:57
Vasanthdev2004
Vasanthdev2004 previously approved these changes May 15, 2026
gnanam1990
gnanam1990 previously approved these changes May 15, 2026
Copy link
Copy Markdown
Collaborator

@gnanam1990 gnanam1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer review of the local Skill Hub CLI slice. I focused on the OpenClaude red-flag surface (network egress, config-path resolution, traversal safety) and treated @Vasanthdev2004's prior install-path review as the deep dive on the install boundary rather than re-deriving it.

Verdict: Approve — non-blocking follow-up requested.

What I independently verified on the current head:

  • Path traversal is genuinely closed: VALID_INSTALL_SKILL_NAME + resolveContainedPath() (relative + .. check) run before any join/rm/cp, temp cleanup targets an explicit tempRoot (not dirname(tempDir)), and there is regression coverage for ../escape from both raw SKILL.md and registry metadata.
  • Network egress is acceptable: fetch() only runs in the explicit skills install registry path (lazy-imported subcommand), never on startup and never in the provider/third-party routing path, and it points at the Gitlawb-owned registry — not an Anthropic/telemetry surface. sha256 pin is mandatory, content is verified before write, min_openclaude_version is enforced, and non-official trust tiers warn.
  • The envUtils.ts change is behavior-preserving: migrateLegacyClaudeConfigHome already early-returns when CLAUDE_CONFIG_DIR is set, so old and new paths resolve identically; the refactor only fixes memoization so a stale default can't mask CLAUDE_CONFIG_DIR (needed for skills install --global). No silent config regression.
  • No tengu_*/fingerprint introduced (the one tengu_ line in the diff is unchanged context), no hardcoded tokens, no CI/workflow diffs.

Non-blocking, please address before/at merge:

  1. The PR description says registry install is "intentionally left for follow-up PRs", but skillsInstall.ts fully implements it including the first network egress for this feature. Please update the summary so the registry-install + network behavior is actually disclosed to anyone reading the PR.
  2. conversationArc.perf.test.ts, knowledgeGraph.stress.test.ts, and providerProfile.test.ts are unrelated CI-stabilization changes bundled into a feature PR — fine to ride here since they were needed for green CI, just calling out the scope.

Good direction and a solid foundation for the Skill Hub. Approving the code; please tidy the description.

Copy link
Copy Markdown
Collaborator

@jatmn jatmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Findings

  • [P2] Local directory installs flatten nested skill namespaces
    src/cli/handlers/skillsInstall.ts:315-332
    src/skills/loadSkillsDir.ts:644-650
    For directory sources, the installer derives the installed name from basename(sourcePath). That breaks any nested skill package whose command name is supposed to come from its path segments. A source directory like repo/git/commit/ now installs into .openclaude/skills/commit, and the loader will expose it as commit, not git:commit. I reproduced this locally with a valid nested skill directory: the handler printed Installing skill "commit" and created only skills/commit. Raw SKILL.md installs preserve names from frontmatter, but the directory install path silently renames nested skills.

  • [P2] The new precedence isolation test is not actually green on Windows
    src/skills/loadSkillsDir.test.ts:164-205
    This test now isolates state by spawning a standalone child process with process.execPath and asserting it exits successfully. In my focused Windows run, that child process exits nonzero before the assertion because importing the source tree in that context fails while resolving src/utils/permissions/yoloClassifier.ts prompt assets (./yolo-classifier-prompts/auto_mode_system_prompt.txt). The rest of the touched tests pass, but this specific new test does not, so the PR's advertised focused validation is not portable yet.

@Vasanthdev2004 Vasanthdev2004 dismissed stale reviews from gnanam1990 and themself via 1a0e39b May 15, 2026 14:45
jatmn
jatmn previously approved these changes May 15, 2026
Copy link
Copy Markdown
Collaborator

@jatmn jatmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up on the earlier review. The local directory install namespace issue looks addressed now, and the Windows-focused loader test is green in my rerun.

No issues here, LGTM.

Vasanthdev2004
Vasanthdev2004 previously approved these changes May 15, 2026
Copy link
Copy Markdown
Collaborator

@techbrewboss techbrewboss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

Thanks for the continued hardening here. The current head looks solid on the earlier install-path and namespace issues, but I found one remaining blocker around the new local skills CLI commands being coupled to provider startup validation.

Findings

  • src/entrypoints/cli.tsx:141 - skills CLI commands still require valid provider startup state.
    Impact: The PR adds local/script-friendly skills management commands, but the entrypoint still runs validateProviderEnvForStartupOrExit() before dispatching skills list/show/validate/install/remove. In non-TTY/script usage, invalid provider env exits before the handler runs, so users cannot inspect or manage local skills until unrelated provider configuration is fixed.

    I reproduced this against the built CLI:

    CLAUDE_CODE_USE_OPENAI=1 OPENAI_BASE_URL=https://api.openai.com/v1 OPENAI_API_KEY= node dist/cli.mjs skills list

    That exits 1 with the OpenAI missing-key error instead of listing local skills.

    Suggested fix: Fast-path args[0] === 'skills' before provider profile hydration/validation, or make startup provider validation skip these local management subcommands. Please also add an entrypoint-level regression test for skills list with invalid provider env and redirected stdout.

Validation

  • bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts
  • bun run build
  • Reproduced the blocker with built dist/cli.mjs
  • bun run security:pr-scan did not reveal a current-head PR blocker from the refreshed diff

@anandh8x anandh8x dismissed stale reviews from Vasanthdev2004 and jatmn via 732be43 May 16, 2026 05:46
@anandh8x
Copy link
Copy Markdown
Collaborator Author

Addressed the final review blocker in commit 732be43.

Changes made:

  • Routed skills CLI commands before provider profile hydration/startup validation so local skills management still works when provider env is broken.
  • Handles supported leading global flags before skills, including openclaude --bare skills list.
  • Added entrypoint regression coverage for both skills list and --bare skills list with invalid OpenAI env.
  • Kept local runtime folders like .omx/ and .codex/ out of the commit.

Verification:

  • bun test src/entrypoints/cli.skills.test.ts src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts
  • bun run build
  • CLAUDE_CODE_USE_OPENAI=1 OPENAI_BASE_URL=https://api.openai.com/v1 OPENAI_API_KEY= node dist/cli.mjs skills list
  • CLAUDE_CODE_USE_OPENAI=1 OPENAI_BASE_URL=https://api.openai.com/v1 OPENAI_API_KEY= node dist/cli.mjs --bare skills list
  • bun run smoke

GitHub Actions are passing on the latest run: smoke-and-tests and web.

Copy link
Copy Markdown
Collaborator

@Vasanthdev2004 Vasanthdev2004 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Targeted re-review of the current head (732be43), focused on the latest startup-validation blocker plus the Skill Hub install/trust boundary.

Verdict: Approve-ready

What I rechecked:

  • skills CLI commands now dispatch before provider profile hydration/startup validation, so local skills management still works when provider env is broken.
  • openclaude skills list and openclaude --bare skills list both work with invalid OpenAI env in the built CLI.
  • install path hardening is still intact: names are normalized before path construction, colon namespaces install as nested paths, temp cleanup uses the explicit temp root, and target paths resolve under the selected skills root before overwrite/copy.
  • registry installs require sha256, verify downloaded SKILL.md before writing, preserve tools_required / min_openclaude_version, and enforce the OpenClaude version gate.
  • no hidden/bidi characters found in changed files, and the PR intent scan is clean.

Verification I ran locally:

  • bun test src/entrypoints/cli.skills.test.ts src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts
  • bun test src/utils/conversationArc.perf.test.ts src/utils/knowledgeGraph.stress.test.ts src/utils/providerProfile.test.ts
  • bun run build
  • bun run smoke
  • bun run security:pr-scan -- --base origin/main
  • git diff --check origin/main...HEAD
  • manual built-CLI checks for skills list and --bare skills list with broken OpenAI env

I do not see a remaining code blocker on the current head.

One non-code cleanup before merge: the PR description is now stale. It still says registry install is left for follow-up, but this PR now implements registry install and introduces explicit registry fetch behavior. Please update the description so the merge record accurately discloses the current scope.

@Vasanthdev2004
Copy link
Copy Markdown
Collaborator

Blockers

None.

Non-Blocking

  • PR description is stale — says registry install is left for follow-up, but this PR now implements it.

Looks Good

  • Adds local skills CLI commands (list, show, validate, remove)
  • Supports project/user/bundled skill discovery with project precedence
  • Install path hardening — names normalized before path construction
  • Registry installs require sha256 verification
  • Skills CLI works when provider env is broken
  • Good test coverage

Verdict: Approve — clean skill CLI addition with proper security hardening.

Vasanthdev2004
Vasanthdev2004 previously approved these changes May 16, 2026
Copy link
Copy Markdown
Collaborator

@Vasanthdev2004 Vasanthdev2004 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clean skill CLI addition with proper security hardening.

@kevincodex1
Copy link
Copy Markdown
Contributor

bro please rebase and fix conflicts

anandh8x added a commit to anandh8x/openclaude that referenced this pull request May 17, 2026
PR Gitlawb#1162 adds local skills discovery and CLI management. Current main already changed startup heap handling and knowledge graph corruption-test isolation, so the merge keeps the skills CLI fast-path while preserving main's unconditional NODE_OPTIONS heap guard and searches corruption artifacts from the resolved Orama path.

Constraint: Resolve PR Gitlawb#1162 against main at f102b60 without dropping either the skills CLI bypass or the mainline heap/knowledge-graph fixes.

Rejected: Keep the PR's older CCR-only heap guard | main intentionally expanded the heap guard for local long-running agents.

Rejected: Search corrupted Orama files from a broad config root | the resolved persistence path is the authoritative location under redirected config state.

Confidence: high

Scope-risk: moderate

Directive: Keep skills management commands available before provider startup validation so broken provider config does not block local skills inspection.

Tested: bun test src/utils/conversationArc.perf.test.ts src/skills/loadSkillsDir.test.ts src/cli/handlers/skills.test.ts src/commands.test.ts src/entrypoints/cli.skills.test.ts src/utils/knowledgeGraph.stress.test.ts

Tested: bun run build

Tested: node dist/cli.mjs --version

Tested: git diff --check
anandh8x and others added 21 commits May 17, 2026 11:58
OpenClaude Skill Hub needs the runtime repo to treat project skills as first-class local assets before registry installation exists. This wires native .openclaude skill directories into discovery, preserves .claude compatibility, and adds list/show subcommands so users can inspect resolved local skills.

Constraint: Keep registry install, website catalog, and community governance out of this first runtime slice.

Rejected: Replace the existing skills loader wholesale | the repo already has working bundled, plugin, MCP, dynamic, and legacy command skill paths.

Confidence: medium

Scope-risk: moderate

Directive: Keep .claude skill loading compatible while .openclaude adoption rolls out.

Tested: bun test src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: node dist/cli.mjs --bare skills list

Tested: node dist/cli.mjs --bare skills show debug

Tested: git diff --check
Skill Hub needs local package hygiene before registry install can be safe. This adds validation for SKILL.md directories and local removal for project or user skills without introducing remote registry behavior yet.

Constraint: Registry install and update flows are still out of scope for this slice.

Rejected: Implement install first | install needs the same validation and local removal semantics to avoid copying unsafe or unmanageable skill folders.

Confidence: medium

Scope-risk: moderate

Directive: Keep validation conservative; loosen individual checks only with explicit registry policy coverage.

Tested: bun test src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: node dist/cli.mjs skills validate .openclaude/skills/demo-skill

Tested: node dist/cli.mjs skills list

Tested: node dist/cli.mjs skills show demo-skill

Tested: node dist/cli.mjs skills remove demo-skill

Tested: git diff --check
Skills management commands are meant to be script-friendly inspection operations. Printing the interactive startup screen before the list/show/validate output makes the command noisy and hard to read.

Constraint: Keep the interactive startup screen for normal OpenClaude sessions.

Confidence: high

Scope-risk: narrow

Tested: bun run build

Tested: node dist/cli.mjs skills list

Tested: git diff --check
The default skills list output was a metadata-heavy dump, which made bundled and local skills difficult to scan. This changes the human formatter to an aligned table with wrapped descriptions while keeping machine-readable metadata behind --json.

Constraint: Default list output must stay compact and human-readable while JSON remains script-friendly.

Rejected: Keep version and trust columns in the default table | those fields add noise and remain available through --json/show.

Confidence: high

Scope-risk: narrow

Directive: Keep the default list formatter focused on scanability; add metadata to --json or detail commands instead of widening the daily table.

Tested: bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: node dist/cli.mjs skills list

Tested: node dist/cli.mjs skills list --json
The PR introduced skills tests that passed in focused runs but failed under the full GitHub Actions Bun test job. The formatter test now uses bun:test consistently, and skill directory tests explicitly restore the setting-source state they rely on.

Constraint: CI runs the full Bun suite, so tests must avoid node:test interop and shared setting-source leakage.

Confidence: medium

Scope-risk: narrow

Tested: bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: git diff --check

Not-tested: Full local bun test still has unrelated provider/OAuth failures on this machine.
The full CI suite can mutate process-wide config state while this test is running, so the user-vs-project precedence assertion now runs in a child Bun process with its own CLAUDE_CONFIG_DIR.

Constraint: getSkillDirCommands reads global config/env state, so this precedence test needs process isolation under the full suite.

Confidence: medium

Scope-risk: narrow

Tested: bun test src/skills/loadSkillsDir.test.ts src/cli/handlers/skills.test.ts src/commands.test.ts

Tested: git diff --check
The CI runner was failing the conversation arc benchmarks because they used shared persisted knowledge graph state and strict wall-clock thresholds. The tests now isolate graph storage in a temporary config directory and keep only coarse regression limits suitable for noisy shared runners.

Constraint: GitHub Actions shared runners can have variable storage/indexing latency.

Rejected: Remove the benchmark coverage entirely | the tests still provide useful regression signals when isolated and coarse-grained.

Confidence: medium

Scope-risk: narrow

Directive: Keep performance tests isolated from persisted user/project graph state.

Tested: bun test src/utils/conversationArc.perf.test.ts src/skills/loadSkillsDir.test.ts src/cli/handlers/skills.test.ts src/commands.test.ts

Tested: bun run smoke
The skill hub CLI could list, inspect, validate, and remove local skills, but it had no supported install path. This adds a project/global install command that accepts local directories, raw SKILL.md files or URLs, and registry IDs with checksum validation when registry metadata provides one.

Constraint: The companion openclaude-skills repository currently publishes SKILL.md files without riskLevel metadata, so validation keeps riskLevel optional while preserving required identity/source fields.

Rejected: Require the external skills repository to be cloned into openclaude | install should work from registry metadata or explicit local paths without coupling the repos.

Confidence: high

Scope-risk: moderate

Directive: Keep --json/list behavior machine-compatible; install output should remain human-readable and validation should not reject normal security-review prose.

Tested: bun test src/cli/handlers/skillsInstall.test.ts src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: bun run smoke
The install tests used shared console and cwd globals, which passed in isolation but raced with unrelated test files under Bun's full parallel suite. This makes the tests assert on installed files directly and injects the project directory into the handler for deterministic test isolation.

Constraint: The CLI still resolves project installs from the runtime cwd; projectDir is only used by direct handler tests.

Confidence: high

Scope-risk: narrow

Tested: bun test src/cli/handlers/skillsInstall.test.ts src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: bun run smoke
The new standalone install test file changed Bun's parallel test scheduling and exposed unrelated global-state races in CI. Moving the coverage into the existing skills handler test file keeps the install behavior covered without adding another parallel test unit.

Constraint: Some existing tests mutate cwd/config globals under full-suite parallelism.

Confidence: medium

Scope-risk: narrow

Tested: bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: git diff --check
The install path used registry or SKILL.md names to create temporary and target directories before validation rejected unsafe names. This validates the install name before path construction, keeps the temp root as an explicit cleanup target, and resolves install targets under the selected skills root before copy or force removal.

Constraint: Registry and raw SKILL.md sources are untrusted until validation completes.

Rejected: Rely on validateSkillPath after temp construction | unsafe names can affect filesystem paths before validation runs.

Confidence: high

Scope-risk: narrow

Tested: bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: bun run smoke

Tested: git diff --check
The default skills list is meant for skills users can inspect and manage in the current environment. Bundled skills remain available internally and in JSON metadata, but the human table now omits bundled rows and removes the Source column.

Constraint: --json remains machine-readable with full source metadata.

Confidence: high

Scope-risk: narrow

Tested: bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: node dist/cli.mjs skills list

Tested: node dist/cli.mjs skills list --json

Tested: git diff --check

Tested: bun run smoke
Tests can mock homedir while leaving CLAUDE_CONFIG_DIR unset, so caching only by the env override can leak a temporary .openclaude root into later config/profile tests. Include homedir in the memoization key so config path resolution follows both inputs.

Constraint: Keep getClaudeConfigHomeDir memoized for hot callers.

Confidence: high

Scope-risk: narrow

Tested: bun test --max-concurrency=1 src/utils/openclaudePaths.test.ts src/utils/providerProfile.test.ts src/utils/knowledgeGraph.stress.test.ts tests/sdk/sdk-context-isolation.test.ts

Tested: bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: node dist/cli.mjs skills list

Tested: bun run smoke

Tested: git diff --check
Bundled skills are internal helpers, so the public skills CLI should only expose installed skills that users can inspect or manage. Filter bundled skills from JSON output and command lookups, and use a generic not-found response for hidden bundled names.

Constraint: Installed project and user skills remain listed, inspectable, removable, and available in JSON.

Confidence: high

Scope-risk: narrow

Tested: bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: node dist/cli.mjs skills list --json

Tested: node dist/cli.mjs skills show batch

Tested: node dist/cli.mjs skills remove batch

Tested: bun run smoke

Tested: git diff --check
The full PR check can load config-path helpers after tests have mocked homedir or changed global session state. Explicit CLAUDE_CONFIG_DIR now bypasses the default-home memoization cache, and SDK contexts now treat sessionProjectDir: null as an intentional context value instead of falling back to stale global state.

Constraint: Keep default config-home resolution memoized for hot callers.

Confidence: high

Scope-risk: narrow

Tested: bun test --max-concurrency=1 src/utils/openclaudePaths.test.ts src/utils/providerProfile.test.ts src/utils/knowledgeGraph.stress.test.ts tests/sdk/sdk-context-isolation.test.ts

Tested: bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: bun run smoke

Tested: git diff --check
The PR check showed provider profile tests sharing process.env/CWD-sensitive state and a knowledge graph stress test assuming a fixed config-root persistence path. Mark the profile tests that mutate global process state as non-concurrent and assert the corrupted Orama rename relative to the actual persistence path under test.

Constraint: Production behavior is unchanged; this only tightens test isolation.

Confidence: high

Scope-risk: narrow

Tested: bun test --max-concurrency=1 src/utils/openclaudePaths.test.ts src/utils/providerProfile.test.ts src/utils/knowledgeGraph.stress.test.ts tests/sdk/sdk-context-isolation.test.ts

Tested: bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: git diff --check
Removing a user-global skill without --global looked like a missing skill even though skills list showed it. Detect when the requested skill exists in the other local scope and print the exact removal command hint while keeping bundled/internal skills hidden as generic not found.

Constraint: Bundled skills remain hidden from public skills commands.

Confidence: high

Scope-risk: narrow

Tested: bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: node dist/cli.mjs skills remove pr-review

Tested: node dist/cli.mjs skills remove batch

Tested: bun run smoke

Tested: git diff --check
The public skills list now hides bundled/internal skills, so an empty result means there are no installed user or project skills. Use clearer copy to avoid implying internal skills do not exist.

Constraint: Bundled skills remain hidden from public skills commands.

Confidence: high

Scope-risk: narrow

Tested: bun test src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: node /home/anaxy/Projects/openclaude/dist/cli.mjs skills list from empty temp project

Tested: bun run smoke

Tested: git diff --check
Skills management commands need to work when provider configuration is broken, because they are local/script-friendly maintenance commands. Route skills subcommands before provider profile hydration and validation, including supported leading global flags such as --bare.

Constraint: Provider startup validation must still run for normal interactive and provider-backed commands.

Rejected: Import full main.tsx for the skills fast path | that loads optional bundled Chrome modules and re-couples the local skills path to interactive startup.

Confidence: high

Scope-risk: narrow

Tested: bun test src/entrypoints/cli.skills.test.ts src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts

Tested: bun run build

Tested: CLAUDE_CODE_USE_OPENAI=1 OPENAI_BASE_URL=https://api.openai.com/v1 OPENAI_API_KEY= node dist/cli.mjs skills list

Tested: CLAUDE_CODE_USE_OPENAI=1 OPENAI_BASE_URL=https://api.openai.com/v1 OPENAI_API_KEY= node dist/cli.mjs --bare skills list

Tested: bun run smoke

Tested: git diff --check
Rebasing PR Gitlawb#1162 onto current main flattened an earlier merge commit that carried reviewed Skill Hub hardening and regression coverage. This restores those final-tree changes as a normal linear commit so the rebased PR keeps the same behavior reviewers approved without retaining merge commits or mainline noise.

Constraint: Keep the PR branch linear for maintainer review while preserving the reviewed final tree from the conflict-resolved integration branch.

Rejected: Push the plain rebase result | it would drop registry sha256/version/trust metadata handling and associated tests from the reviewed PR state.

Confidence: high

Scope-risk: narrow

Directive: Do not remove the registry sha256 requirement or install-path regression tests without another security review.

Tested: final tree compared against fix-pr-1162-conflicts before verification
@anandh8x anandh8x force-pushed the skill-hub-local-skills-cli branch from 6a9d30a to 4ff411d Compare May 17, 2026 06:33
@gnanam1990
Copy link
Copy Markdown
Collaborator

Thanks for the rebase and the focused follow-ups. I reviewed the three new commits against the standing blockers and they line up well:

  • Provider independence (2e1bb06): getSkillsCliArgs() now fast-paths skills … into runSkillsCli before validateProviderEnvForStartupOrExit(), including handling of leading boolean/value flags. This addresses @techbrewboss's blocker — local skill management no longer requires valid provider env.
  • Namespace preservation (c79a1d0): directory installs now resolve the name via getSkillNameFromDirectory() (SKILL.md frontmatter) and skillNameToInstallPath() maps git:commitgit/commit. This addresses @jatmn's flattening point for the common case.
  • Path-traversal hardening (4ff411d): install/target paths still go through resolveContainedPath(), so the containment guard from the earlier review survived the rebase (@Vasanthdev2004's blocker).

Two things to confirm before this is fully clear:

  1. The namespace fix relies on SKILL.md frontmatter carrying the namespaced name; getSkillNameFromDirectory() still falls back to basename(sourcePath), so a nested directory without a namespaced frontmatter name would still flatten. Is that the intended contract (frontmatter is the source of truth)? A short note/test for the fallback case would make it explicit.
  2. loadSkillsDir.ts isn't touched by these commits — could you confirm the loader exposes the new nested install path (skills/git/commit) as git:commit end-to-end? @jatmn's original finding was two-part (installer + loader); the installer half is fixed, want to be sure the loader half resolves too.

One note on process: given the size (21 files / ~2.2k lines), I verified the three fix commits specifically rather than re-running the full suite in-session — worth confirming CI / bun test + bun x tsc --noEmit are green on the current head before merge. The targeted fixes look right; nice work.

Copy link
Copy Markdown
Collaborator

@Vasanthdev2004 Vasanthdev2004 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Targeted re-review of the rebased current head (4ff411d), focused on whether the previously reviewed Skill Hub hardening survived the rebase.

Verdict: Approve-ready

What I rechecked:

  • registry installs still require sha256 and verify the downloaded SKILL.md before writing anything
  • min_openclaude_version is still enforced at install time
  • tools_required and min_openclaude_version are still preserved into installed skill.json
  • install path hardening survived the rebase: names are normalized before path construction, unsafe/path-like names are rejected, temp cleanup uses the explicit temp root, and target paths resolve under the selected skills root
  • namespaced local installs like git:commit still install as nested paths and load back with the namespace intact
  • skills CLI commands still bypass provider startup validation, including openclaude --bare skills list
  • PR intent scan is clean and CI is green

Verification I ran locally:

  • bun test src/entrypoints/cli.skills.test.ts src/cli/handlers/skills.test.ts src/skills/loadSkillsDir.test.ts src/commands.test.ts
  • bun run build
  • bun run smoke
  • bun run security:pr-scan -- --base origin/main
  • git diff --check origin/main...HEAD
  • manual built-CLI checks for skills list and --bare skills list with broken OpenAI env

I do not see a remaining code blocker on the rebased head.

Non-blocking before merge: the PR description is still stale. It says registry install is left for follow-up, but this PR now includes registry install and registry network fetch behavior. Please update the body so the merge record matches the actual scope.

@Vasanthdev2004
Copy link
Copy Markdown
Collaborator

@kevincodex1 merge this regarding skills update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants